if this change doesn't land before workspace config is released there should be a migration like for repo config here.

I figured that even if it was released by then it'd be so recently released that my guess is that only you, as the PR author, would probably be the only person who has workspace config.

I can do this if need be, but I'm not convinced it's worth the effort.

I haven't read the code yet, but if workspace-config.toml complicates the stuff, I think it's okay to remove the workspace config layer at all. Maybe we can add --when.workspace_names = ["default"] if --when.workspaces = ["/path/to/workspace"] is tedious to write.

if it's an option, I'd prefer we temporarily disable workspace configs for the next release if this code hasn't landed by then, to avoid introducing more tech debt. I think it's probably ok to wait one extra release for the per-workspace configs.

I don't have issues with per-workspace configs, just don't want to have to go through the same migration process for them that we do for per-repo configs to migrate to this new format.

temporarily disabling or requiring manual migration seems fine to me.

I don't think anyone is saying that workspace config isn't useful. We can all agree that we want the feature enabled, I just think we should delay the release of the feature until this PR lands.

My main concern with leaving it in for the current release and adding migration code is that it adds another corner case that needs to be handled for a very long time, so unless we consider the feature urgent, I think it's worth delaying.

I personally consider that workspace config can be replaced with repo config + conditionals. I don't mean the workspace config is useless, but I don't think it would be worth the effort/complexity to add two separate config + signing file pairs. (It would be nice if we didn't have repo/workspace config files that could be a security risk, but removing all of them wouldn't be acceptable.)

If we're unsure whether we'll re-add workspace config, temporary removal would be good. If we're sure to support workspace config, I think manual migration (without temporary removal) is good enough.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

I personally consider that workspace config can be replaced with repo config + conditionals.

I don't think this is true for all the people who want to run their coding agents in separately configured workspaces. Since they want a one-time setup for the agents environment.

I don't mean the workspace config is useless, but I don't think it would be worth the effort/complexity to add two separate config + signing file pairs. (It would be nice if we didn't have repo/workspace config files that could be a security risk, but removing all of them wouldn't be acceptable.)

Agreed, having two signed pairs kinda complicates all of this.

If we're sure to support workspace config, I think manual migration (without temporary removal) is good enough.

I also agree with that. Although having an automated way would improve the user experience.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

Isn't this technically a bug? Because otherwise this seems to be a really bad oversight.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

Isn't this technically a bug? Because otherwise this seems to be a really bad oversight.

I mean it's not obvious whether the repo-managed config should be considered a repo-level or not. If it were "workspace-managed", user's .jj/repo/config.toml would be overridden by workspace-managed config. This wouldn't be what the user would want (unless they use workspace-config.toml extensively.)

I've actually been having discussions with my team about managed repo config, and they raised some very good points that made me completely reconsider how it should work. Specifically, we have a fake "repo-managed" config currently which is just a symlink to an in-repo config file in chromium.

The issue they've been running into is that they've been doing things like jj new <LTS branch> to cherry-pick certain fixes onto said branch. When they do so, because the LTS branch is old, they don't have any configuration. This also results in very confusing UX when one workspace has one version of the jj config checked out while another workspace has a different version. In general, the current approach:

• Increases implementation complexity
• Increases complexity from a user's perspective
  • They might get it out of sync whenever they run jj new
  • They could have one alias work when you're in one place, then suddenly disappear in a different place.

The approach we think would work better is actually to just always read the repo-managed config from trunk(). AFAICT the pros and cons are:

• Pro: Much simpler to implement
• Pro: Much simpler from a user's perspective
• Pro: Consistent user experience between versions
• Cannot test new configurations easily, since it reads only submitted configs
  • Can be mitigated by adding a global flag to read the config from @ instead. This would bypass trust, and thus mean that they don't have to review their own configs that they wrote.
• Pro: Any references to out-of-repo tools will actually work with old versions
• Con: Any references to in-repo tools may break if you move the tool
  • The same applies if the tool didn't exist at an old version, but the previous approach also wouldn't solve that problem

AFAICT, the only downside can be relatively easily mitigated by just not moving tools, or moving tools but adding symlinks, etc.

The priority list would thus be:

• User
• Repo-managed
• Repo
• Workspace

further up i mentioned the workspace-config.toml, should we update that too?

I'll leave this open, depends on whether we land this before a new release.

workspace config has the upside that you don't have to write absolute paths compared to --when.workspaces. to me having the workspace config seems like a useful feature, but i'm not too fussed as my usecases are also covered by when (but i'm not sure about the other people who were pushing for workspace config).

do we think it's likely this pr doesn't land soon? do we think workspace config is not a useful feature?

if workspace config is good and this pr doesn't land soon, we can mention it as experimental in the release notes and require manual migration once this pr lands?

do we think workspace config is not a useful feature?

If it was requested by multiple users for multiple use-cases it should stand on its own as a useful feature (which it was). I mean @HybridEidolon's initial use-case was a good example of it uses. I also don't like disabling it when the migration provided here doesn't consider it yet.

do we think it's likely this pr doesn't land soon?

I think this needs some further discussion and I don't expect this go in faster than other stuff still pending.

nit: remove the whitespace before the period

I personally consider that workspace config can be replaced with repo config + conditionals.

I don't think this is true for all the people who want to run their coding agents in separately configured workspaces. Since they want a one-time setup for the agents environment.

I don't mean the workspace config is useless, but I don't think it would be worth the effort/complexity to add two separate config + signing file pairs. (It would be nice if we didn't have repo/workspace config files that could be a security risk, but removing all of them wouldn't be acceptable.)

Agreed, having two signed pairs kinda complicates all of this.

If we're sure to support workspace config, I think manual migration (without temporary removal) is good enough.

I also agree with that. Although having an automated way would improve the user experience.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

Isn't this technically a bug? Because otherwise this seems to be a really bad oversight.

nit: squash this and the nix commit into the first one (although you may need a Nix expert to help you with the problems there before doing that).

Does this mean new config is no longer a plain text file? I thought there would be config.toml which is still readable, and a separate file containing signature.

Deleting config.toml would break forward compatibility, so it would require grace period. Even after the grace period, I prefer plain text format because the file can be inspected/edited by using ordinary tools. For example, we can use less .jj/repo/config.toml to review someone else's repository configuration without trusting it.

Does this mean new config is no longer a plain text file? I thought there would be config.toml which is still readable, and a separate file containing signature.

That's correct. I thought long and hard about this, and flip-flopped a few times, but decided that this was my preferred approach.

Deleting config.toml would break forward compatibility, so it would require grace period.

I have already done this. It automatically migrates from the old config to the new one.

Even after the grace period, I prefer plain text format because the file can be inspected/edited by using ordinary tools. For example, we can use less .jj/repo/config.toml to review someone else's repository configuration without trusting it.

You can run jj config edit --repo to review someone else's repository config without trusting it (since their repo config won't be enabled initially). And if you don't trust it, you delete the parts you don't like (or the whole thing) so it's safe.

I'm not personally a huge fan of the idea of having config files on disk at all, and I actually significantly prefer the new UX. If we train users to "view / edit your config file by editing .jj/repo/config.toml", that will provide a very inconsistent experience.

• If a user has multiple workspaces, this only works from the main workspace
• Technically, it is not stored at .jj/repo/config.toml. It is stored at $REPO_DIR/config.toml, and $REPO_DIR is usually .jj/repo. If the repo dir for a particular repo is stored at a different location, it will not work.
• This doesn't work from subdirectories of the repo root. This usually isn't too much of a problem, but for users at google working on our monorepo, for example, the .jj directory isn't where most users would expect (it's actually at ../.jj/repo)
• The user now has two ways of editing the config file (jj config edit --repo still works).

IMO, since jj config edit --repo is so much more reliable, I much prefer the consistency of:

• To edit repository config, run jj config edit --repo
• To read repository config, run jj config edit --repo
  • We could potentially add jj config show --repo down the line, similar to jj file show if we want to be able to interface with standard tools to read the config, but I simply don't see a strong use case for it. IMO, tools should never need to interface directly with the config files given the existence of jj config get/set/unset/list

The only possible advantage I can think of to keep the file around and edit it directly is that this would allow you to edit the config file in an IDE easily instead of in the terminal.

The thing I don't like about keeping the config file is that by allowing the user to edit the file directly, we are unable to sign the contents of the file. We need a mechanism to tie the signature to the file content, which means jj needs to manage the writing of the content. My personal preference is to keep it as is, as the user can still inspect the config file via jj config edit --repo, and we could easily add jj config show --repo which would write it to disk.

I can see a few different options to solve this problem:

Option 1

We keep the config files, but don't read from them and make them readonly. When you run jj config edit --repo, it would write back the contents to disk as a readonly file.

In the event that the secure config doesn't exist, we simply write the current config to the secure config, then copy it back to the old config path as readonly (this is the migration path).

I don't like anything about this TBH, I don't think it improves on anyway over the existing mechanism

Option 2

Rather than attempting to sign the contents of the config file, we attempt to sign whether we want to enable the config file. We change the proto to:

message RepoConfig {
  bool enable_repo_config = 1;
}

message WorkspaceConfig {
  bool enable_workspace_config = 1;
}

It's not as secure as my current approach, but I believe it would still solve the specific attack vector that is the zip file problem. I think that if we were to change approach, this is the one we should switch to.

This seems to go way out of the way to special case this file, which I thought went against this project's policies.
I don't quite see how you have a config file in the repo without having a file. Wouldn't all the commands that deal with files have to special case it?

I don't quite see how you have a config file in the repo without having a file.

You have a file, it's just that the file, roughly speaking, instead of being a string representing the configuration, contains something more akin to the struct:

struct SecureConfig {
  config: String,
  // sign(config, my credentials)
  signature: Vec<u8>
}

So it is stored in a file, it's just that the file isn't directly edited via a text editor, any modifications are wrapped by jj config edit

Wouldn't all the commands that deal with files have to special case it?

No, because this isn't stored in the repo. No commands read / write the config file (other than jj config, which I've already done in this PR)

No, because this isn't stored in the repo.

I thought storing the config in the repo was the whole point. We talked before about switching to different commits that didn't have it, and it was never a file in the repo?

I thought storing the config in the repo was the whole point. We talked before about switching to different commits that didn't have it, and it was never a file in the repo?

It's stored in the repo dir, which is $REPO/.jj/repo/config. That directory is "in the repo" in the sense that it is contained within the directory that is the repository, but it is not tracked by version control, and is thus unaffected by jj new, for example.

It sounds like you may also be mixing this up with a seperate discussion about what I've come to call "managed repo config", which is stored in $REPO/.config/jj/config.toml and thus stored in version control. That is mostly unrelated to this particular discussion.

nit: Perhaps, this doesn't have to be abstracted as a trait. Maybe we can use parameter struct something like struct { kind: &'static str, filename: &'static str }?

I don't think we need public-key cryptography to sign and verify user's private data by his key. What we need is something like hash(private_key + path + config) but is considered secure. I think some MAC algorithm can be used. RustCrypto lists hmac crate in this category.

https://en.wikipedia.org/wiki/Message_authentication_code
https://github.com/RustCrypto

I'm not an expert, but pubkey-based algorithm would be computationally more expensive than symmetric-key algorithms or cryptographic hashing.

I double-checked with a security expert, they say that you're right and that asymmetric keys aren't improving anything.

They also mentioned that specifically, "HMAC_SHA256 would be a sensible choice"

nit: Perhaps, path resolution (and/or key loading) should be done by non-jj-lib code, and provided as a function parameter? It will help write library tests.